FM2-653 #556

mogoodrich · 2025-01-09T21:26:24Z

Description of what I changed

Issue I worked on

https://openmrs.atlassian.net/browse/FM2-653

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/BaseDao.java

mogoodrich · 2025-01-09T21:29:05Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirLocationDaoImpl.java

@@ -38,6 +41,8 @@
 @Setter(AccessLevel.PACKAGE)
 public class FhirLocationDaoImpl extends BaseFhirDao<Location> implements FhirLocationDao {

+	private static final int SUPPORTED_LOCATION_HIERARCHY_SEARCH_DEPTH = 9;


Basically, I use a succession of joins to go back up the ancestor tree. A bit annoying/hacky, but from a practical matter, seems unlikely there would ever be a hierarchy of more than 9 levels. We could make this a configurable parameter if we wanted.

Maybe we turn this into a GP? That way an implementation can adjust it to the actual depth of their location hierarchy.

Fair enough, I will do that (and since it's configurable, set a more normal default of like 5)

mogoodrich · 2025-01-09T21:31:08Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirLocationDaoImpl.java

+	private void handleAncestorLocation(Criteria criteria, ReferenceAndListParam ancestor) {
+
+		List<Criterion> elementOrAncestorEqualsReferenceCriteria = new ArrayList<>();
+		Optional<Criterion> elementEqualsReferenceCriterion = handleLocationReference(ancestor); // note: "below" is inclusive of the element itself


see: https://hl7.org/fhir/R4/search.html#recursive

mogoodrich · 2025-01-09T22:23:18Z

api/src/main/java/org/openmrs/module/fhir2/providers/r3/LocationFhirResourceProvider.java

@@ -113,6 +113,9 @@ public IBundleProvider searchLocations(@OptionalParam(name = Location.SP_NAME) S
 	        @OptionalParam(name = Location.SP_PARTOF, chainWhitelist = { "", Location.SP_NAME, Location.SP_ADDRESS_CITY,
 	                Location.SP_ADDRESS_STATE, Location.SP_ADDRESS_COUNTRY,
 	                Location.SP_ADDRESS_POSTALCODE }, targetTypes = Location.class) ReferenceAndListParam parent,
+	        @OptionalParam(name = "below", chainWhitelist = { "", Location.SP_NAME, Location.SP_ADDRESS_CITY,


So it sounds like @ibacher this extra parameter should not be here, and this new functionality should just be part of the "SP_PARTOF" parameter above? I was having a problem with that, see my follow on comment and screenshot in the test case below.

The tl;dr is that if I change this to name="partof:below" any parameter passed in that way gets assigned to both the ReferenceAndListParam ancestor and ReferenceAndListParam parent.

And if I remove the "below" parameter, I'm not quite sure how to parse the "parent" ReferenceAndListParam to tell if this should be a standard "partof" vs a "partof:below"

this extra parameter should not be here, and this new functionality should just be part of the "SP_PARTOF" parameter above?

Ideally, yes.

And if I remove the "below" parameter, I'm not quite sure how to parse the "parent" ReferenceAndListParam to tell if this should be a standard "partof" vs a "partof:below"

That's very problematic. The problem is that partof=123abc&partof:below=456def is technically valid and I'm not quite sure what HAPI would do with that...

mogoodrich · 2025-01-09T22:28:46Z

.../java/org/openmrs/module/fhir2/providers/r3/LocationFhirResourceProviderIntegrationTest.java

@@ -483,6 +486,31 @@ public void shouldSearchForExistingLocationsAsJson() throws Exception {
 		assertThat(entries, containsInRelativeOrder(hasResource(hasProperty("name", equalTo("Test location 6"))),
 		    hasResource(hasProperty("name", equalTo("Test location 8")))));
 		assertThat(entries, everyItem(hasResource(validResource())));
+
+		// search by ancestors
+		response = get("/Location?below=" + LOCATION_ANCESTOR_TEST_UUID).accept(FhirMediaTypes.JSON).go();


So when I changed this to "Location?partof:below=" it was getting picked up by the ResourceParam, but the "below" was being set to the ResourceType. This seems incorrect? Isn't the type of this resource a location?

mogoodrich · 2025-01-17T21:23:31Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirLocationDaoImpl.java

-		handleLocationReference("loc", parent).ifPresent(loc -> criteria.createAlias("parentLocation", "loc").add(loc));
+	private void handleLocationReference(Criteria criteria, ReferenceAndListParam locationAndReferences) {
+
+		if (locationAndReferences == null) {


All this through 148 is basically a series null / size checks to make sure that ReferenceAnd list only contains a single reference. We probably should change this parameter to just a ReferenceParam but I didn't want to untangle the locationReference utility method at this point.

Basically, the "partof:below" functionality I built currently only works correctly for a single parameter (and I don't have the need to build more now). Technically the plain "partof" works for multiple "partof", but would always return null since a location partof is 0..1

mogoodrich · 2025-01-23T14:33:04Z

@ibacher sorry, just another ping on this, can you take a look when you get a chance? This is basically updating things based on what we discussed when you were in Boston last week.

ibacher

Thanks @mogoodrich! I think this basically looks as good as we can get it!

ibacher · 2025-01-24T18:31:44Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirLocationDaoImpl.java

+
+			// we need to add a join to the parentLocation for each level of hierarchy we want to search, and add "equals" criterion for each level
+			int depth = 1;
+			while (depth <= SUPPORTED_LOCATION_HIERARCHY_SEARCH_DEPTH) {


Should this be a for loop?

You mean aesthetically, or is there a bug here I'm missing? :) For some reason I tend to prefer while loops.

Just aesthetics. It looks like it works.

ibacher · 2025-01-24T18:32:18Z

api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirLocationDaoImpl.java

@@ -38,6 +41,8 @@
 @Setter(AccessLevel.PACKAGE)
 public class FhirLocationDaoImpl extends BaseFhirDao<Location> implements FhirLocationDao {

+	private static final int SUPPORTED_LOCATION_HIERARCHY_SEARCH_DEPTH = 9;


Maybe we turn this into a GP? That way an implementation can adjust it to the actual depth of their location hierarchy.

mogoodrich · 2025-01-24T20:38:39Z

@ibacher updated to make search depth a global property as you suggested. Will merge assuming all tests pass.

# Conflicts: # api/src/test/java/org/openmrs/module/fhir2/MockedGlobalPropertyServiceConfiguration.java

mogoodrich · 2025-01-24T22:33:24Z

Merging, thanks @ibacher !

mogoodrich added 5 commits January 9, 2025 09:33

FM2-653: Add support for searching for Locations by ancestor

5b94f18

FM2-653: Add support for searching for Locations by ancestor

c46ba07

FM2-653: Add support for searching for Locations by ancestor

e5b00bd

FM2-653: Add support for searching for Locations by ancestor

9319b83

FM2-653: Add support for searching for Locations by ancestor

90ec5dd

mogoodrich commented Jan 9, 2025

View reviewed changes

mogoodrich requested review from ibacher and mseaton January 9, 2025 21:40

mogoodrich commented Jan 9, 2025

View reviewed changes

FM2-653: Add support for searching for Locations by ancestor

29a5cb0

mogoodrich commented Jan 17, 2025

View reviewed changes

mogoodrich added 2 commits January 17, 2025 16:26

FM2-653: Add support for searching for Locations by ancestor

47d35d7

FM2-653: Add support for searching for Locations by ancestor

19723f0

ibacher approved these changes Jan 24, 2025

View reviewed changes

FM2-653: Add support for searching for Locations by ancestor

b1efbfb

mogoodrich added 3 commits January 24, 2025 15:56

FM2-653: Add support for searching for Locations by ancestor

be996fe

Merge branch 'master' into FM2-653

370d392

# Conflicts: # api/src/test/java/org/openmrs/module/fhir2/MockedGlobalPropertyServiceConfiguration.java

FM2-653: Add support for searching for Locations by ancestor

aafa52a

mogoodrich merged commit 0e04589 into master Jan 24, 2025
2 checks passed

mogoodrich deleted the FM2-653 branch January 24, 2025 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FM2-653 #556

FM2-653 #556

mogoodrich commented Jan 9, 2025 •

edited

Loading

mogoodrich Jan 9, 2025

ibacher Jan 24, 2025

mogoodrich Jan 24, 2025

mogoodrich Jan 9, 2025

mogoodrich Jan 9, 2025

mogoodrich Jan 9, 2025

ibacher Jan 13, 2025

mogoodrich Jan 9, 2025

mogoodrich Jan 17, 2025 •

edited

Loading

mogoodrich commented Jan 23, 2025

ibacher left a comment

ibacher Jan 24, 2025

mogoodrich Jan 24, 2025

ibacher Jan 24, 2025

ibacher Jan 24, 2025

mogoodrich commented Jan 24, 2025

mogoodrich commented Jan 24, 2025

FM2-653 #556

FM2-653 #556

Conversation

mogoodrich commented Jan 9, 2025 • edited Loading

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogoodrich Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

mogoodrich commented Jan 23, 2025

ibacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mogoodrich commented Jan 24, 2025

mogoodrich commented Jan 24, 2025

mogoodrich commented Jan 9, 2025 •

edited

Loading

mogoodrich Jan 17, 2025 •

edited

Loading